Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up rstar versioned dependencies #759

Merged
merged 6 commits into from
Mar 16, 2022
Merged

Clean up rstar versioned dependencies #759

merged 6 commits into from
Mar 16, 2022

Conversation

nyurik
Copy link
Member

@nyurik nyurik commented Mar 8, 2022

Explicitly declare rstar_0_8 and rstar_0_9 to make it clearer which version is being used. For backward compatibility, the rstar is now a feature that enables rstar_0_8

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Explicitly declare `rstar_0_8` and `rstar_0_9` to make it clearer which version is being used.  For backward compatibility, the `rstar` is now a feature that enables `rstar_0_8`

TBD: does this need a changelog message, and if so, what kind of text is expected?
use-rstar = ["rstar", "approx"]
# Prefer `use-rstar` feature rather than enabling rstar directly.
# rstar integration relies on the optional approx crate, but implicit features cannot yet enable other features.
# See: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#namespaced-features
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#namespaced-features

Namespaced features has been stabilized in the 1.60 release.

Nice! So in 4 releases (~24 weeks) we could think about utilizing them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(unrelated to your PR, I know... the diff just caused me to check the link)

@michaelkirk
Copy link
Member

bors r+

thanks!

bors bot added a commit that referenced this pull request Mar 10, 2022
759: Clean up rstar versioned dependencies r=michaelkirk a=nyurik

Explicitly declare `rstar_0_8` and `rstar_0_9` to make it clearer which version is being used.  For backward compatibility, the `rstar` is now a feature that enables `rstar_0_8`

TBD: does this need a changelog message, and if so, what kind of text is expected?

- [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md).
- [ ] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---



Co-authored-by: Yuri Astrakhan <[email protected]>
@michaelkirk
Copy link
Member

bors r-

Oh, right, the changelog...

Your GH message seems pretty close to a useful changelog message. I'd recommend proposing something based on that.

@bors
Copy link
Contributor

bors bot commented Mar 10, 2022

Canceled.

@lnicola
Copy link
Member

lnicola commented Mar 10, 2022

Umm, the plan was to drop the plain rstar feature in the next minor release, right?

@michaelkirk
Copy link
Member

Umm, the plan was to drop the plain rstar feature in the next minor release, right?

Can you clarify what you mean by "minor" - it's a bit confusing because geo-types is 0.x.y

In #682 (comment) you and @rmanoka discussed it, and it seems like you agreed to remove it in the next breaking release.

@lnicola
Copy link
Member

lnicola commented Mar 10, 2022

Next minor as in 0.20.0 (0.19.1 would be a patch release).

What I mean to say is that if we're expecting to merge soon other breaking changes, it's not worth doing this. If we plan to put out a 0.19.1 at some point, then it's fine.

@michaelkirk
Copy link
Member

What I mean to say is that if we're expecting to merge soon other breaking changes, it's not worth doing this.

What other breaking changes are we planning to merge soon?

Can you clarify what you'd like to see happen with this PR @lnicola? I'm a bit confused because you approved it, but now it seems like you're saying you'd prefer not to merge it.

@nyurik
Copy link
Member Author

nyurik commented Mar 10, 2022

My goal for this PR was purely a cleanup without any visible changes. We can later figure out if we want any breaking or non-breaking changes, if anything should be removed/renamed/...

@nyurik
Copy link
Member Author

nyurik commented Mar 10, 2022

Your GH message seems pretty close to a useful changelog message. I'd recommend proposing something based on that.

thx @michaelkirk , done

@lnicola
Copy link
Member

lnicola commented Mar 10, 2022

What other breaking changes are we planning to merge soon?

I don't think I've checked every open PR, but #751 is breaking AFAICT.

Can you clarify what you'd like to see happen with this PR @lnicola? I'm a bit confused because you approved it, but now it seems like you're saying you'd prefer not to merge it.

No preference, I was just pointing out that we might have to update this again (and remove it form the changelog) if we merge another breaking change before the next release. Even in that case, this PR is fine, which is why I approved it.

@nyurik
Copy link
Member Author

nyurik commented Mar 10, 2022

Thx @lnicola , makes sense. The #751 is a bit raw at the moment, so no need to merge it just yet until we are clear we really need it. The #760 and #761 are a bit more important to me, as they make it easier to introduce breaking changes as needed, and simplify their reviews.

geo-types/CHANGES.md Outdated Show resolved Hide resolved
@michaelkirk
Copy link
Member

Thanks for clarifying @lnicola - sometimes I have a hard time understanding people on the internet.

Co-authored-by: Michael Kirk <[email protected]>
@nyurik nyurik requested a review from michaelkirk March 16, 2022 02:51
@nyurik
Copy link
Member Author

nyurik commented Mar 16, 2022

Are there any blockers for this noop PR? Seems like all comments have been resolved?
P.S. (mostly noop) :)

@lnicola
Copy link
Member

lnicola commented Mar 16, 2022

The changes here are good. I think we should also drop the deprecated feature, but that's a different discussion.

bors r+

@nyurik
Copy link
Member Author

nyurik commented Mar 16, 2022

@lnicola #772

@nyurik
Copy link
Member Author

nyurik commented Mar 16, 2022

@lnicola I think your bors-foo is not as strong as @michaelkirk 's ;)

@lnicola
Copy link
Member

lnicola commented Mar 16, 2022

@nyurik
Copy link
Member Author

nyurik commented Mar 16, 2022

So you broke github too? Nice :)

@bors
Copy link
Contributor

bors bot commented Mar 16, 2022

Build succeeded:

@bors bors bot merged commit b6bfff1 into georust:master Mar 16, 2022
@nyurik nyurik deleted the rstar branch March 16, 2022 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants